@claude review the PR

Claude finished @hanwencheng's task in 2m 10s —— View job

PR Review: refactor(session_store): thread base_dir + KeyringMode through API (#34)

Verdict: Approve with suggestions — This is a well-motivated, clean refactor. The core design is sound, and the test migration is the right call over the ctor+serial_test approach in #42. A few issues worth addressing before merge.

• Read changed files
• Analyze session_store.rs changes
• Analyze cli/lib.rs changes
• Analyze cli_tests.rs changes
• Post review feedback

Bugs / Correctness

1. Redundant self.load() at the end of load_with_legacy_fallback

crates/agentkeys-core/src/session_store.rs:242

pub fn load_with_legacy_fallback(&self, session_id: &str) -> Result<Session> {
    if let Ok(s) = self.load(session_id) {
        return Ok(s);  // Fast path — already failed here
    }
    if session_id == "master" {
        // ...legacy paths...
    }
    self.load(session_id)  // ← Will always fail; same as the initial load
}

After the initial self.load fails and all legacy fallbacks are exhausted, calling self.load again is redundant — it will produce the same error as the first call, just with a less useful message (no mention of the legacy paths that were tried). Replace with a direct Err:

Err(anyhow::anyhow!("no session found for id={session_id} (and no legacy fallback matched)"))

Fix this →

2. list_ids returns sanitized names — round-trip list_ids → load silently double-sanitizes for unusual session IDs

crates/agentkeys-core/src/session_store.rs:194-215

Directory names on disk are the sanitized forms. list_ids returns them verbatim, but load(returned_id) re-applies sanitize_for_keyring — meaning any session ID that was rewritten (e.g. foo/bar → __agk_foo_bar-cc5d46bd) would be double-sanitized and the load would fail. In practice this is harmless because all real session IDs (master, daemon-0xABC, etc.) are valid under the sanitizer. But this is a latent correctness hazard. At minimum add a comment:

// NOTE: returned names are the on-disk (sanitized) forms. For all production
// session IDs (alphanumeric + dash) sanitize_for_keyring is a no-op, so these
// can be passed directly to load(). If you ever call list_ids on a store that
// might hold rewritten IDs, be aware of this.

Design Concerns

3. session_store() re-reads env vars on every call

crates/agentkeys-cli/src/lib.rs:110-114

pub fn session_store(&self) -> SessionStore {
    self.session_store_override
        .clone()
        .unwrap_or_else(SessionStore::from_env)  // reads $HOME on every call
}

cmd_revoke calls ctx.session_store() twice in the same function body (once for each branch). from_env re-reads $HOME and AGENTKEYS_SESSION_STORE each time and allocates a new PathBuf. This is cheap in wall-clock terms, but it means the effective store can differ between calls within a single command if the env is mutated concurrently (unlikely, but semantically surprising).

The simpler fix is to resolve from_env once at construction time and store it in session_store_override unconditionally:

pub fn new(backend_url: &str, verbose: bool, json_output: bool) -> Self {
    Self {
        // ...
        session_store_override: Some(SessionStore::from_env()),  // eager, once
    }
}

Then with_session_store just overrides it, and session_store() always returns a &SessionStore reference (or clone of the stored value). This removes the conceptual split between "override" and "default" entirely.

4. session_store_override is pub while a builder pattern exists

crates/agentkeys-cli/src/lib.rs:54

pub session_store_override: Option<SessionStore>,

External code can bypass the builder and directly mutate or inspect this field. Since the other override fields (session_override, backend_override) are also pub, this is consistent with the existing pattern — but it's worth considering pub(crate) for all three to make the builder the sole external API.

Style / Minor

5. _session naming in start_scope_test_server — used variable should not have leading underscore

crates/agentkeys-cli/tests/cli_tests.rs:799

let (_output, _session) = cmd_init(...).await.unwrap();
// ...
.header("authorization", format!("Bearer {}", _session.token))

_session is actively used on the very next line. Leading underscores conventionally signal "intentionally unused." Rename to session to match intent.

Fix this →

6. Legacy free functions have no deprecation marker

crates/agentkeys-core/src/session_store.rs:326-359

The PR preserves fallback_path, save_session, load_session, etc. as thin wrappers and the docs say "new code should prefer building a SessionStore explicitly." A #[deprecated] annotation would enforce this at the compiler level and prevent new callers from accidentally using the old API:

#[deprecated(since = "0.2.0", note = "Use SessionStore::from_env().session_path() instead")]
pub fn fallback_path(session_id: &str) -> PathBuf {
    SessionStore::from_env().session_path(session_id)
}

What's Done Well

• The isolation-by-construction invariant (file_only ↔ custom root, Auto ↔ home root only) is clearly documented and enforced structurally. The comment at lines 62–71 is the right level of explanation for a subtle correctness property.
• Test hermetic isolation is now correct: each test owns its TempDir and SessionStore, no shared global state, no #[serial] required.
• Path traversal defense (sanitize_for_keyring + session_dir routing through it) is sound and well-tested.
• Legacy wrappers preserving the existing daemon/CLI interface is the right call for a refactor of this scope.
• The clear synchrony fix (keyring delete blocks before returning) and the list_ids sort are important correctness properties that survive the refactor intact.